Now I have everything I need for a complete review. Here is my full assessment:

Review: feat(cli,format): deterministic JSON envelope with plumb_version, run_id, summary

Crates touched: plumb-format only.

1. Determinism Invariants

PASS. No wall-clock sources, no HashMap/HashSet in output paths. serde_json::Map with explicit alphabetical insertion is the right defensive move given the transitive serde_json/preserve_order activation via schemars. The run_id derivation is correct: compact-serialize the sorted violations array (not the pretty envelope), then SHA-256 hex-encode — so whitespace tweaks and version bumps never shift the digest. Defensive re-sort before hashing is correct.

sha2 = "0.10" is declared in [workspace.dependencies] and is MIT-licensed (in the deny.toml allowlist). Layer is clean: plumb-format → plumb-core + external only.

2. Workspace Layering

PASS. #![forbid(unsafe_code)] at src/lib.rs:18. No internal cross-crate direction violations.

3. Error Handling

PASS. All serde_json calls use ?. No unwrap/expect in library code (#![deny(clippy::unwrap_used, clippy::expect_used)] at src/lib.rs:20 enforces this).

4. Test Coverage — Issues

Warning 1 — dead test; the reordering invariant is never actually exercised.

crates/plumb-format/tests/determinism.rs:89:

if violations.len() < 2 {
    return; // canned fixture too small to reorder
}

The golden snapshot at format_snapshots__json.snap:11 confirms the canned fixture produces exactly 1 violation ("total": 1). So violations.len() < 2 is always true and the reordering body never runs. The test json_run_id_is_stable_under_input_reordering is permanently silent — it never asserts anything.

Fix options (pick one):

• Replace the fixture() call in this specific test with a hand-built Vec<Violation> of length ≥ 2, as is done in other determinism tests.
• Change return to panic!("canned fixture must produce ≥ 2 violations for this test to be meaningful") so a fixture regression is loud.

Warning 2 — plumb_version is a volatile field baked into a golden snapshot.

crates/plumb-format/tests/snapshots/format_snapshots__json.snap:6:

"plumb_version": "0.0.1",

Every routine version bump (0.0.1 → 0.0.2) will fail just test with an insta diff that has zero content significance. The testing rule (.agents/rules/testing.md) explicitly covers this: "insta::Settings can redact volatile fields — use it for paths and timings when you can't avoid them." plumb_version is in the same category as a path or timestamp.

Fix: in format_snapshots.rs, wrap the json_snapshot test with:

let mut settings = insta::Settings::clone_current();
settings.add_redaction(".plumb_version", "[version]");
settings.bind(|| insta::assert_snapshot!("json", out));

The json_envelope_has_required_fields test in determinism.rs already checks !plumb_version.is_empty() structurally, which is the right place for that invariant. No value in also locking it in a snap file.

5. Documentation

PASS. json() has thorough doc including # Envelope, # run_id derivation, and # Errors. PLUMB_VERSION has an explanatory comment. hex_digest (private) is slightly confusingly named — it takes bytes-to-hash, not a pre-computed digest — but it's private and the comment clarifies it. Minor nit, not actionable.

Punch List

Verdict: REQUEST_CHANGES